-
Notifications
You must be signed in to change notification settings - Fork 352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: warn, exit and error utils from oclif command class #3028
Conversation
59c2eb4
to
3103edb
Compare
@erezrokah Please review the PR. |
Hi @tinfoil-knight, sorry for the delay. The approach for creating these utilities is nice! The only concern is for them to change in the base class, so our implementation will drift. However I don't think it happens very often (without it being a breaking change). Since this PR touches many files, it will take some time to review as I would like to invoke each code flow to make sure the change works (not all flows are covered by tests). |
That sounds reasonable. Thank you for the response. |
@tinfoil-knight, sorry for the delay to review this. Then we can gradually introduce the usage of those methods into different areas of the codebase, and we won't need to keep this PR up to date with other changes. Please let me know what you think |
Don't worry about it.
Yeah. That seems like a good idea. I'll pick this and the other sub-PRs up then in some time. P.S. Although, unless it's a substantial testing/review issue, I can keep the PR up to date every few days until you can review it. |
Started with #3247 |
The apology is on our side @tinfoil-knight for the delay reviewing your contribution. Thanks a lot for doing all of this! ❤️ Also, college work always prevails! :) Erez is on PTO so I'm going to help with reviewing your awesome contribution (which I find very helpful!). If you want to keep breaking this down into separate PRs, I'll make sure to review each of them as soon as possible. 👍 |
Thank you a lot. |
- Summary
Ref: #2728
Some logging and process control utilities (
log
,warn
,exit
,error
) were being passed down to each method and helper functions which isn't ideal. Thus,log
was refactored in #2827 &warn
,exit
&error
have been refactored in this PR.The utilities have been referenced from the
@oclif/command
package to not break any existing functionality. They are present in thesrc/utils/command-helpers.js
file.- Test plan
npx ava --verbose --serial